Skip to content

Conversation

@dminuoso
Copy link

@dminuoso dminuoso commented Aug 25, 2018

This fix is similar to:
#178

The existing code has overlapping instances, namely for:

(l :+: r) and (C1 (...) :+: r)

The second instance is unnecessary, since the instance for C1 (...) (S1 sa sb) will produce a type error already.

Some code that triggers the overlapping instances error:

{-# LANGUAGE DeriveGeneric    #-}
{-# LANGUAGE TypeApplications #-}

import GraphQL.Internal.API.Enum (GraphQLEnum(..))
import GHC.Generics (from, to, Generic)

data Magic = Left Int | Right deriving (Eq, Show, Generic)

instance GraphQLEnum Magic

main = main

f = enumValues @Magic

Also an unary constructor is one that takes one argument, so I took the liberty to rename the diagnostics to nullary constructors.

A test case would be a bit more difficult because it still fails compilation.

@teh
Copy link
Collaborator

teh commented Aug 25, 2018

Looks good, thank you! To make the test pass can you adjust COVERAGE_TOLERANCE like in this diff?

@dminuoso
Copy link
Author

@teh Done

Copy link
Collaborator

@jml jml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Needs a rebase and a small change.

genericEnumToValue = nonUnaryConstructorError

nonUnaryConstructorError :: a
nonUnaryConstructorError = panic "Tried to construct enum with non-unary constructor. Should get a compile-time error instead of this."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also rename this function to nonNullaryConstructorError. We don't have to worry about backwards compatibility as it is not exported and in Internal anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants